Skip to content

Conversation

@Arshia-r-m
Copy link
Contributor

The test added shows an unexpected behavior of the nodes after the 2nd node swapped (bought) 2 assets with the 1st node and then tried to swapped these assets with the 1st node again.

@nicbus
Copy link
Member

nicbus commented Sep 29, 2025

Swap quantities (qty_from, qty_to) behave the opposite way, as documented for MakerInitRequest:

// "from" and "to" are seen from the taker's perspective, so:
// - "from" is what the taker will send and the maker will receive
// - "to" is what the taker will receive and the maker will send

Therefore, when swapping from 5 asset 1 to 5 asset 2:

  • the maker will send 5 asset 2 and receive 5 asset 1
  • the taker will send 5 asset 1 and receive 5 asset 2

The expected off-chain balances after the last swap in the test then are:

  • maker: 595 asset 1, 585 asset 2
  • taker: 5 asset 2, 15 asset 2

With these balances the test passes.

That being said, I've observed the test still failing sometimes due to balances not updating after the last swap (asset 1 remaining 590/10), will look into that.

PS: a dedicated issue_second_asset_nia helper isn't needed, the existing one (issue_asset_nia) can be used for both assets; even if ticker and name are the same, the asset ID will be unique so they behave as two distinct assets.

@Arshia-r-m
Copy link
Contributor Author

Thanks for the clear explanations, the expected amounts after the last swap is wrong and I'll modify it, but the fact that the the balances aren't updated remains the same.

The test tries to replicate the exact situation I faced multiple times in my testing where the maker marks the swap as failed, taker marks it as pending and the amounts will not change.

Let me know if I could provide more details.

@nicbus
Copy link
Member

nicbus commented Sep 29, 2025

Thanks for the clear explanations, the expected amounts after the last swap is wrong and I'll modify it, but the fact that the the balances aren't updated remains the same.

From some local test runs I did, with the correct balances the test passes most of the time.
As I was mentioning in my previous comment, it doesn't work 100% of the time, which is odd and we'll be looking into that.

The test tries to replicate the exact situation I faced multiple times in my testing where the maker marks the swap as failed, taker marks it as pending and the amounts will not change.

From this I get that you expect the test to fail because of the maker failing the swap, is my understanding correct?
Anyway, will do more local runs and check for this kind of behavior.

@Arshia-r-m
Copy link
Contributor Author

From this I get that you expect the test to fail because of the maker failing the swap, is my understanding correct? Anyway, will do more local runs and check for this kind of behavior.

To be exact, from my understanding, the maker marks the swap as failed because it fails to pay to taker, so the swap it self is marked as failed, but on the taker side it is stuck in pending status and also the balances doesn't change on both end.
I'll update the test asap and push it again.

@Arshia-r-m Arshia-r-m force-pushed the impl-assett-to-asset-swap branch from e95b0fb to 1d6bcd1 Compare September 29, 2025 14:45
@nicbus
Copy link
Member

nicbus commented Oct 13, 2025

To be exact, from my understanding, the maker marks the swap as failed because it fails to pay to taker, so the swap it self is marked as failed, but on the taker side it is stuck in pending status and also the balances doesn't change on both end.

Both maker and taker mark the swap as succeeded, but the balance fails to update to the expected values because the second part of the swap sometimes happens on the wrong channel.

The underlying cause of the bug has been identified and fixed.

We're going to merge this test and will then improve on it to add more checks.

Before we merge it, though, we'd like for the name of the test to be changed into swap_assets_liquidity_both_ways (including the name and directory inside the test file) and the commit to be signed. Can you please apply these changes?

@Arshia-r-m
Copy link
Contributor Author

Of-course, thanks for resolving this.

@Arshia-r-m Arshia-r-m force-pushed the impl-assett-to-asset-swap branch from 96bd94b to 3bf7f89 Compare October 13, 2025 17:25
@Arshia-r-m Arshia-r-m force-pushed the impl-assett-to-asset-swap branch from 3bf7f89 to cfeb4ba Compare October 13, 2025 17:30
@zoedberg zoedberg merged commit cfeb4ba into RGB-Tools:master Oct 14, 2025
12 checks passed
@zoedberg
Copy link
Member

Merged, thanks!

@Arshia-r-m Arshia-r-m deleted the impl-assett-to-asset-swap branch October 16, 2025 12:32
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants